feat: implement endpoint to support fetching workspace pod log - #1267
feat: implement endpoint to support fetching workspace pod log#1267YoyinZyc wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
f184804 to
fcb68f7
Compare
0496f5d to
f30cafe
Compare
|
/ok-to-test |
… workspace pod log Signed-off-by: Yuchen Zhou <yczhou@google.com>
… level unit tests Signed-off-by: Yuchen Zhou <yczhou@google.com>
Signed-off-by: Yuchen Zhou <yczhou@google.com>
Signed-off-by: Yuchen Zhou <yczhou@google.com>
andyatmiami
left a comment
There was a problem hiding this comment.
@YoyinZyc - Sorry I've been distracted by life - and took me awhile to be able to dig into this PR.
Overall very excited about the implementation here - a lot of "nitpicks" around various things that is completely natural this being your first pR..
But the core implementation is really well done
Let me know if you disagree with any of my comments and want to talk about them - always happy to discuss!
| // HTTP: 404 with a caller-provided message. | ||
| func (a *App) notFoundResponseWithMessage(w http.ResponseWriter, r *http.Request, err error) { | ||
| httpError := &HTTPError{ | ||
| StatusCode: http.StatusNotFound, | ||
| ErrorResponse: ErrorResponse{ | ||
| Code: strconv.Itoa(http.StatusNotFound), | ||
| Message: err.Error(), | ||
| }, | ||
| } | ||
| a.errorResponse(w, r, httpError) | ||
| } | ||
|
|
There was a problem hiding this comment.
Open to discussion - but I'm not sure this function is really warranted.. all existing endpoints use the existing a.notFoundResponse(w, r) and I think that works well enough as is..
Would prefer to keep with convention here - but let me know if I am overlooking something and/or why you think we should add this...
| if err != nil { | ||
| switch { | ||
| case errors.Is(err, repository.ErrWorkspaceNotFound): | ||
| a.notFoundResponseWithMessage(w, r, err) |
There was a problem hiding this comment.
Related to: https://github.com/kubeflow/notebooks/pull/1267/changes#r3695881990
I think we should just stick with a.notFoundResponse(w, r) to "keep it simple"
| var ( | ||
| ErrWorkspaceNotFound = fmt.Errorf("workspace not found") | ||
| ErrPodNotRunning = fmt.Errorf("workspace pod is not running") | ||
| ErrContainerNotFound = fmt.Errorf("container not found in pod") |
There was a problem hiding this comment.
| ErrContainerNotFound = fmt.Errorf("container not found in pod") | |
| ErrContainerNotFound = fmt.Errorf("container not found in workspace pod") |
| logsContainerQueryParam = "container" | ||
| logsTailLinesQueryParam = "tailLines" | ||
| logsPreviousQueryParam = "previous" | ||
| logSinceTimeQueryParam = "sinceTime" |
There was a problem hiding this comment.
| logSinceTimeQueryParam = "sinceTime" | |
| logsSinceTimeQueryParam = "sinceTime" |
| It("should return 409 when the workspace pod is not running", func() { | ||
| By("creating the HTTP request") | ||
| req, ps := buildLogsRequest(namespaceName, workspaceName, "") | ||
|
|
||
| By("executing GetWorkspaceLogsHandler") | ||
| rr := httptest.NewRecorder() | ||
| a.GetWorkspaceLogsHandler(rr, req, ps) | ||
| rs := rr.Result() | ||
| defer rs.Body.Close() | ||
|
|
||
| By("verifying status is 409 Conflict") | ||
| Expect(rs.StatusCode).To(Equal(http.StatusConflict)) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The 409 test ("workspace pod is not running") only checks the status code:
Expect(rs.StatusCode).To(Equal(http.StatusConflict))
But the 404 test in this same file goes further — it parses the ErrorEnvelope and asserts the error message matches the sentinel. The 409 test should do the same for consistency within the file as well as generally being much more reliable/robust/accurate.
| if opts.Previous && apierrors.IsBadRequest(err) && strings.Contains(err.Error(), "previous terminated container") { | ||
| return nil, ErrPreviousLogsNotFound | ||
| } | ||
| return nil, fmt.Errorf("failed to open log stream for pod %s, container %s: %w", podName, containerName, err) |
There was a problem hiding this comment.
seems like we can at least define the "template string" as a const at the top of this file - so its easier to see/locate all our error strings together..
| // None requested: default to the primary (first regular) container. | ||
| if len(podStatus.Containers) == 0 { | ||
| return "", "", ErrContainerNotRunning | ||
| } | ||
| containerName = podStatus.Containers[0].Name |
There was a problem hiding this comment.
🤔 I wonder if we capitalize on the fact we hardcode main as the container name for our primary container - and in this branch "force" that as the default...
i guess defensively we'd still want to ensure there IS a main container to be robust against the unknown
| // if the target container is still in the Waiting state (i.e. it has not started yet | ||
| // and therefore has no logs available for the current instance). | ||
| func (r *LogsRepository) ensureContainerStarted(ctx context.Context, namespace, podName, containerName string) error { | ||
| pod, err := r.clientset.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) |
There was a problem hiding this comment.
This is probably implied from the function comment:
the live Pod status
... but might be nice to also explicitly call out here we are opting to use the clientset (vs. client) as we are (presumably) trying to make a real-time decision on state and want to avoid a potentially stale cached version
| // The Workspace status references a pod that no longer exists. | ||
| return ErrPodNotRunning | ||
| } | ||
| return fmt.Errorf("failed to get pod %s: %w", podName, err) |
There was a problem hiding this comment.
seems like we can at least define the "template string" as a const at the top of this file - so its easier to see/locate all our error strings together..
| for _, group := range [][]corev1.ContainerStatus{pod.Status.ContainerStatuses, pod.Status.InitContainerStatuses} { | ||
| for _, cs := range group { | ||
| if cs.Name != containerName { | ||
| continue | ||
| } | ||
| // A container that is still Waiting has never started and has no logs yet. | ||
| if cs.State.Waiting != nil { | ||
| return ErrContainerNotRunning | ||
| } | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
While admittedly not a realistic "performance concern" - I personally like the "more boring" two-loop style iteration present here:
its a simple linear scan with no extra allocations...
but at minimum - I think we should be consistent in iteration logic implementation
christian-heusel
left a comment
There was a problem hiding this comment.
Great work @YoyinZyc this is already quite awesome 🔥
I looked through the code a bit and left a review below! Feel free to apply / challenge / ignore it as needed 🤗
| // The number of lines to retrieve from the end of the logs. | ||
| // By default, the value is 1000. | ||
| TailLines int64 |
There was a problem hiding this comment.
This does not currently seem to be respected:
$ curl -sk -H "Kubeflow-Userid: admin" https://localhost:8443/workspaces/api/v1/workspaces/default/jupyterlab-workspace/podtemplate/logs/batch\?tail=10 | wc -l
42
There was a problem hiding this comment.
Ah nevermind, this was just due to me using the parameter naming:
$ curl -sk -H "Kubeflow-Userid: admin" https://localhost:8443/workspaces/api/v1/workspaces/default/jupyterlab-workspace/podtemplate/logs/batch\?tailLines=10 | wc -l
10
Maybe we should guard against wrongly spelt parameter? 🤔 Anyways, that's most likely out of scope for this PR 😅
Also see https://github.com/kubeflow/notebooks/pull/1267/changes#r3708558872
| logsContainerQueryParam = "container" | ||
| logsTailLinesQueryParam = "tailLines" | ||
| logsPreviousQueryParam = "previous" | ||
| logSinceTimeQueryParam = "sinceTime" |
There was a problem hiding this comment.
I think those should potentially live in api/constants/query_params.go:
notebooks/workspaces/backend/api/constants/query_params.go
Lines 17 to 20 in 97159cb
| func parseLogOptions(r *http.Request) (*models.LogOptions, field.ErrorList) { | ||
| var valErrs field.ErrorList | ||
| query := r.URL.Query() | ||
|
|
||
| opts := &models.LogOptions{ | ||
| Container: query.Get(logsContainerQueryParam), | ||
| } | ||
|
|
||
| if raw := query.Get(logsTailLinesQueryParam); raw != "" { | ||
| tail, err := strconv.ParseInt(raw, 10, 64) | ||
| if err != nil || tail <= 0 { | ||
| valErrs = append(valErrs, field.Invalid(field.NewPath(logsTailLinesQueryParam), raw, "must be a positive integer")) | ||
| } else { | ||
| opts.TailLines = tail | ||
| } | ||
| } | ||
|
|
||
| if raw := query.Get(logsPreviousQueryParam); raw != "" { | ||
| previous, err := strconv.ParseBool(raw) | ||
| if err != nil { | ||
| valErrs = append(valErrs, field.Invalid(field.NewPath(logsPreviousQueryParam), raw, "must be a boolean")) | ||
| } else { | ||
| opts.Previous = previous | ||
| } | ||
| } | ||
|
|
||
| if raw := query.Get(logSinceTimeQueryParam); raw != "" { | ||
| t, err := time.Parse(time.RFC3339, raw) | ||
| if err != nil { | ||
| valErrs = append(valErrs, field.Invalid(field.NewPath(logSinceTimeQueryParam), raw, "must be a valid RFC3339 timestamp")) | ||
| } else { | ||
| sinceTime := metav1.NewTime(t) | ||
| opts.SinceTime = &sinceTime | ||
| } | ||
| } | ||
|
|
||
| return opts, valErrs | ||
| } |
There was a problem hiding this comment.
I think to more closely follow the codebase conventions we could potentially introduce 3 new helper functions and add them to internal/helper/validation.go:
func ValidateFieldIsPositiveInt64(path *field.Path, value string) (int64, field.ErrorList)
func ValidateFieldIsBool(path *field.Path, value string) (bool, field.ErrorList)
func ValidateFieldIsRFC3339Time(path *field.Path, value string) (metav1.Time, field.ErrorList)Also we could validate the container name:
if raw := query.Get(logsContainerQueryParam); raw != "" {
valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Label(field.NewPath(logsContainerQueryParam), raw)...)
opts.Container = raw
}| case errors.Is(err, repository.ErrPodNotRunning): | ||
| a.conflictResponse(w, r, err, nil) |
There was a problem hiding this comment.
Is a conflictResponse really the right thing to return here? 🤔
I'm asking because this is also what is returned in case a workspace is paused:
$ curl -k -H "Kubeflow-Userid: admin" https://localhost:8443/workspaces/api/v1/workspaces/default/jupyterlab-workspace/podtemplate/logs/batch
{"error":{"code":"409","message":"workspace pod is not running","cause":{}}}
| // When previous=true but the container has never restarted, the Kubernetes | ||
| // API returns a 400 with a "previous terminated container ... not found" | ||
| // message. Surface this as a semantic error instead of a generic 500. | ||
| if opts.Previous && apierrors.IsBadRequest(err) && strings.Contains(err.Error(), "previous terminated container") { |
There was a problem hiding this comment.
I looked a bit into alternatives for it and since we already fetch the pod we can also do something like this (🤖 -generated; just want to showcase the idea):
diff --git a/workspaces/backend/internal/repositories/logs/repo.go b/workspaces/backend/internal/repositories/logs/repo.go
index f8a7917b..b37afd20 100644
--- a/workspaces/backend/internal/repositories/logs/repo.go
+++ b/workspaces/backend/internal/repositories/logs/repo.go
@@ -20,7 +20,6 @@ import (
"context"
"fmt"
"io"
- "strings"
kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1"
corev1 "k8s.io/api/core/v1"
@@ -87,12 +86,6 @@ func (r *LogsRepository) OpenLogStream(ctx context.Context, namespace, workspace
stream, err := req.Stream(ctx)
if err != nil {
- // When previous=true but the container has never restarted, the Kubernetes
- // API returns a 400 with a "previous terminated container ... not found"
- // message. Surface this as a semantic error instead of a generic 500.
- if opts.Previous && apierrors.IsBadRequest(err) && strings.Contains(err.Error(), "previous terminated container") {
- return nil, ErrPreviousLogsNotFound
- }
return nil, fmt.Errorf("failed to open log stream for pod %s, container %s: %w", podName, containerName, err)
}
return stream, nil
@@ -145,47 +138,52 @@ func (r *LogsRepository) resolvePodAndContainer(ctx context.Context, namespace,
containerName = podStatus.Containers[0].Name
}
- // When requesting current (not previous) logs, ensure the target container has
- // actually started by inspecting the live Pod status. A container still in the
- // Waiting state (e.g. PodInitializing, ContainerCreating, ImagePullBackOff) has
- // no current log stream yet, and the Kubernetes API would return an opaque error;
- // surface it as a semantic 409 instead. Previous logs are exempt, since a
- // terminated instance can have logs even while the current instance is Waiting.
- if !opts.Previous {
- if err := r.ensureContainerStarted(ctx, namespace, podName, containerName); err != nil {
- return "", "", err
+ // Inspect the live Pod status to determine whether the requested log stream can
+ // actually exist. Deciding this up front from the typed status lets us return a
+ // semantic error instead of interpreting the opaque error the Kubernetes API
+ // would otherwise return.
+ cs, err := r.findContainerStatus(ctx, namespace, podName, containerName)
+ if err != nil {
+ return "", "", err
+ }
+ if opts.Previous {
+ // A previous instance exists only if the container has terminated at least
+ // once. This holds even while the current instance is Waiting (e.g. a
+ // container in CrashLoopBackOff still has logs from its last run).
+ if cs.LastTerminationState.Terminated == nil {
+ return "", "", ErrPreviousLogsNotFound
}
+ } else if cs.State.Waiting != nil {
+ // A container still in the Waiting state (e.g. PodInitializing,
+ // ContainerCreating, ImagePullBackOff) has never started, so it has no log
+ // stream for the current instance yet.
+ return "", "", ErrContainerNotRunning
}
return podName, containerName, nil
}
-// ensureContainerStarted checks the live Pod status and returns ErrContainerNotRunning
-// if the target container is still in the Waiting state (i.e. it has not started yet
-// and therefore has no logs available for the current instance).
-func (r *LogsRepository) ensureContainerStarted(ctx context.Context, namespace, podName, containerName string) error {
+// findContainerStatus returns the live status of the named container, searching both
+// the regular and init container statuses of the pod.
+func (r *LogsRepository) findContainerStatus(ctx context.Context, namespace, podName, containerName string) (*corev1.ContainerStatus, error) {
pod, err := r.clientset.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
// The Workspace status references a pod that no longer exists.
- return ErrPodNotRunning
+ return nil, ErrPodNotRunning
}
- return fmt.Errorf("failed to get pod %s: %w", podName, err)
+ return nil, fmt.Errorf("failed to get pod %s: %w", podName, err)
}
- // Search both regular and init container statuses for the target container.
for _, group := range [][]corev1.ContainerStatus{pod.Status.ContainerStatuses, pod.Status.InitContainerStatuses} {
- for _, cs := range group {
- if cs.Name != containerName {
- continue
- }
- // A container that is still Waiting has never started and has no logs yet.
- if cs.State.Waiting != nil {
- return ErrContainerNotRunning
+ for i := range group {
+ if group[i].Name == containerName {
+ return &group[i], nil
}
- return nil
}
}
- return ErrContainerNotRunning
+ // The container is declared in the pod spec but has no status yet, so it has not
+ // started and has no logs.
+ return nil, ErrContainerNotRunning
}
Implement the batch log API proposed in #886 (comment)
closes: #887
related: #886
Behavior
A new one-shot endpoint returning a workspace pods container(including the initContainer like istio) logs as a plain text.
Query params: container(defaults to first/primary container); tail(default to 1000); previous (default to false); sinceTime(default to none)
API
Response
Unit tests
internal/repositories/logs/repo_test.goworkspace_logs_handler_test.goTested Manually
BASE_URL=https://localhost:8443/workspaces/api/v1/workspaces/default/jupyterlab-workspace/podtemplate/logs/batch
Positive
Negative